js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow - #34246
Conversation
|
Reproduced with c05d57f adds a CI on 3638fd1 (build 73378): |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Checked #17561: it does not reproduce as |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The two-token lookahead that lets "async as T" parse as a cast also stops the
arrow commit on "for (async of [7])", so add the missing [lookahead != async of]
restriction in t_for instead of relying on the arrow path to reject it. Matches
esbuild's diagnostic; "for await (async of ...)" and "for (async of => {};;)"
stay accepted.
Only set bad_async_range when the next token is literally "of". Casts and the non-null "!" are erased by parse_suffix without wrapping the expression, so the previous "bare EIdentifier" check incorrectly rejected "for (async as any of x)" and "for (async! of x)".
There was a problem hiding this comment.
All three earlier review rounds have been addressed (c05d57f added the for (async of guard, 3dddc6f trimmed the comments, caafbca tightened the guard to the literal async of token sequence). I didn't find anything further, but this is parser grammar code that took a couple of iterations to get right, so worth a quick human pass.
What was reviewed:
- Traced the new lookahead through
parse_async_prefix_exprforasync as/satisfies/in/!,async ident =>, andexport default async as T— all resolve correctly and JS-mode behavior is unchanged. - Verified
bad_async_rangenow keys onis_contextual_keyword(b"async") && next-token-is-"of"(raw-text match, so escaped spellings andasync.x/async!/async as Tare not caught) and is cleared when the init becomes an arrow (async of => {}). - Checked
for await (async ofstays accepted,for (async\nofstays rejected, and the snapshot/restore +is_log_disabledpattern matches the existing lexer backtracking helpers.
Extended reasoning...
Overview
The PR fixes a TypeScript-mode parse failure where an identifier literally named async followed by as/satisfies/in was misparsed as the start of an async arrow. The fix adds a one-token lookahead in parse_async_prefix_expr (mod.rs) so async <ident> only commits to the arrow path when the token after <ident> is =>, matching tsc's isUnParenthesizedAsyncArrowFunctionWorker and esbuild's port of it. Because that lookahead removed the accidental rejection of for (async of ...), a dedicated bad_async_range guard was added in t_for (parse_stmt.rs) alongside the existing bad_let_range, and after a follow-up round it now keys on the literal two-token sequence async of via check_for_of_after_the_current_token. Tests in transpiler.test.js cover the positive cases, the arrow-still-works case, the for-of rejection, and the for-of edge cases (for await, async.x, async as T, async!, async of => {}).
Security risks
None. This is a syntactic disambiguation in the TS parser; no I/O, allocation-size, or trust-boundary changes. The lexer snapshot/restore pattern used for lookahead is the same one already used elsewhere for TS backtracking, and is_log_disabled is saved/restored so a failing speculative next() doesn't leak diagnostics.
Level of scrutiny
Moderate-to-high. js_parser is core, hot-path, ported-from-esbuild code where subtle grammar interactions are easy to miss — this PR itself needed two follow-up commits after review to close the for (async of hole and then to stop over-rejecting for (async as T of / for (async! of. The final shape diverges structurally from esbuild (which threads an exprFlagForLoopInit into parseAsyncPrefixExpr and checks ident == "of" there) in favor of a bad_let_range-style guard in t_for; after caafbca the two are functionally equivalent, but a maintainer may have a preference on which pattern to keep for future esbuild-diff maintainability.
Other factors
All three of my prior inline threads are resolved and the fixes match what was requested. The bug hunter found nothing on the current head. Test coverage is good and includes the negative case (ts.expectParseError("for (async of [7]);", ...)), the still-an-arrow case (async as => ...), and the for-await exemption. I traced the remaining edge I could think of — for await (async of outside an async context, where is_for_await is reset to false before the guard — and it produces a cascading second error on already-invalid input, which is harmless. Given the change is narrow and now matches reference behavior, this is close to approvable, but parser changes that required multiple correction rounds are worth a human sanity check before merge.
next_token_matches(pred) owns the snapshot/advance/restore sequence; restore() already resets is_log_disabled so drop the redundant manual save/restore. Also lock in "for (async\nof x);" being rejected in both JS and TS mode: the guard is not TS-gated, which matches V8 and the [lookahead != async of] spec restriction (esbuild accepts because its check sits inside the no-newline block).
There was a problem hiding this comment.
The final revision addresses all three earlier findings and I found nothing new, but I'd like a human to sign off given this touches the core JS parser and intentionally changes JS-mode behavior (for (async\nof x) now rejected — spec-correct, but user-visible).
Checked: the next_token_matches snapshot/restore round-trips is_log_disabled; is_contextual_keyword compares raw() so the escaped-\u0061sync and for await carve-outs hold; bad_async_range is keyed on the literal async of token pair and cleared for the async of => {} arrow case; JS-mode for (async of x) still errors via the old arrow-commit path.
Extended reasoning...
Overview
The PR fixes a TypeScript-mode parse failure where async used as a plain identifier followed by as T / satisfies T was misparsed as the start of an async arrow. It touches src/js_parser/parse/mod.rs (adds next_token_matches lookahead helper and gates the T::TIdentifier arm of parse_async_prefix_expr on it in TS mode), src/js_parser/parse/parse_stmt.rs (adds a bad_async_range guard in t_for mirroring the existing bad_let_range pattern), and adds ~20 test cases in test/bundler/transpiler/transpiler.test.js.
This is the fourth review pass. The prior three rounds surfaced (1) a spec-compliance regression where for (async of x) became accepted in TS mode, (2) a comment-length nit, and (3) the bad_async_range guard over-matching async as T / async! in for-of position. All three are resolved in the current diff (commits c05d57f, 3dddc6f, caafbca, 3638fd1) and each has dedicated test coverage.
Security risks
None. This is grammar-level disambiguation in the parser with no I/O, no allocation-size arithmetic on untrusted data, and no auth/crypto involvement. The lookahead uses the existing snapshot()/restore() lexer machinery, which is Copy-based and already used for other backtracking paths.
Level of scrutiny
High — the JS parser runs on every file Bun bundles or executes, and grammar edge cases here have a track record of subtle interactions (this PR itself needed three rounds to get the for-of guard right). The change is small and matches both reference implementations (tsc's isUnParenthesizedAsyncArrowFunctionWorker and esbuild's df815ac), which reduces risk considerably, but it also introduces an intentional behavior change in JS mode: for (async\nof x); was previously accepted (the newline skipped the arrow commit) and is now rejected with the new diagnostic. That is spec-correct and matches V8, but it is a user-visible tightening outside the TS-only scope of the headline fix, and worth a maintainer glance.
Other factors
- Test coverage is thorough: as/satisfies/in, arrow-still-works (
async as => ...), statement-level andexport defaultpositions, and the full for-of matrix (for await,async.x,(async),async as T,async satisfies T,async!,async of => {}, newline, escaped spelling) in both loaders. - The
bad_async_rangeguard now keys on the literal token pair viais_contextual_keyword(b"async") && next_token_matches(is_contextual_keyword(b"of")), which matches the spec's two-token lookahead restriction exactly and avoids the parsed-shape false positives from the earlier revision. - I verified
LexerSnapshotcapturesis_log_disabled, so the helper'sis_log_disabled = trueis unconditionally restored; andis_contextual_keywordcomparesraw(), so\u0061synccorrectly bypasses the guard. - In JS mode without a newline,
for (async of x)still errors via the pre-existing arrow-commit path (Expected "=>") before the new guard is reached, so the existing.toThrow()test at transpiler.test.js:2272 remains satisfied for the same reason as before.
|
@robobun is the token lookahead smart so it remembers the future token for the next scan() and doesn't scan it twice (if it goes down that codepath)? |
|
No, it is not smart in that sense. This is the same trade-off esbuild makes: its The restore cannot be skipped on the arrow path because the arm immediately reads |
|
yeah it would be best if we avoid lexing that identifier twice, code like that is common. can you investigate and determine if this is feasible? |
Capture the identifier and its loc before advancing, then advance once. In TS mode the advance is speculative (log disabled, snapshot restored when the next token is not "=>"), so the common "async x => ..." path now scans "=>" once instead of twice. JS mode is unchanged: it advances once and commits, exactly as before the lookahead was introduced.
|
Investigated and it is feasible without a lexer pushback slot. Pushed in 985542c. What was getting scanned twice was the token after the identifier (so
The remaining |
|
@robobun if the token lexed twice is the token after the identifier then we should probably continue to do this because it's not expensive and it's most likely simpler code. please update it if this is true |
…path" This reverts commit 985542c.
|
That is true, reverted in b7f57b1. The token that was being lexed twice is the one after the identifier (so |
* upstream/main: (57 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (70 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ... # Conflicts: # test/js/bun/websocket/websocket-server.test.ts
2 participants
Problem
A variable or parameter literally named
asynccannot be followed by a TypeScriptas/satisfiescast:Both
tscand esbuild accept this and emitg(async).Cause
parse_async_prefix_exprcommits toasync <ident> => ...as soon as it sees any identifier afterasync, then fails inparse_arrow_bodywhen=>is not the next token. TypeScript's parser resolves this ambiguity with a two-token lookahead (isUnParenthesizedAsyncArrowFunctionWorker, microsoft/TypeScript#8444), and esbuild ported the same lookahead in evanw/esbuild@df815ac for evanw/esbuild#4027.Fix
In TypeScript mode, before committing to
async ident => ..., snapshot the lexer, advance one token, and check whether it is=>. If it is, parse the async arrow as before. If it is not, fall through and treatasyncas a plain identifier so the suffix parser can handleas/satisfies/in/etc. A newnext_token_matcheshelper owns the snapshot/advance/restore sequence.Because the lookahead removes the accidental rejection of
for (async of [7]),t_fornow enforces the[lookahead != async of]grammar restriction directly via abad_async_rangeguard next to the existingbad_let_range, keyed on the literalasyncoftoken sequence. That guard runs in both JS and TS mode, sofor (async\nof x);is now rejected withFor loop initializers cannot start with "async of"in JS mode too (it was previously accepted there); this matches V8 and the spec.for await (async of ...),for (async of => {};;),for (async.x of ...),for ((async) of ...),for (\u0061sync of ...),for (async as T of ...)andfor (async! of ...)are all still accepted.Verification
New cases in
test/bundler/transpiler/transpiler.test.jscoverasync as T,async satisfies T,async in x,async as => ...(still an arrow with parameteras), statement-level andexport defaultpositions, a realasync x => ..., thefor (async ofrejection in both loaders (single-line and with a newline), and the for-of edge cases listed above. The new test block fails on the current release (Expected "=>" but found "boolean") and passes with this change; the rest oftranspiler.test.js(172 pass) is unaffected.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file